EffOperations #7764
EffOperations #7764
Conversation
Absolutionism
requested review from
APickledWalrus and
Efnilite
and removed request for
a team
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/lang/arithmetic/Arithmetics.java
Outdated
Show resolved
Hide resolved
|
|
||
| test "operations effect invalid operation": # TODO: test for runtime errors | ||
| set {_date} to now | ||
| multiply {_date} by 2 |
There was a problem hiding this comment.
should be asserting the value of {_date} afterwards
| error("Cannot operate with a null object."); | ||
| return; | ||
| } | ||
| if (left.isSingle() && left.getSingle(event) == null) { | ||
| error("Cannot operate on a null object."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Shouldn't be doing errors here imo. Precedent is for effects acting on nothing to simply do nothing.
There was a problem hiding this comment.
I might like to see us experiment with having this piggyback off of ExprArithmetic. That is, you create a new ExprArithmetic and initialize it with the associated matched pattern and such. You can then just evaluate the expression in execute and change the value. If there is some major reason this wouldn't (or doesn't) work, let me know. OR, it might be better to just pull out some of the shared logic for unparsed literal/return type handling
Also, it might not hurt to have an arithmetic module :) would like to hear what others think about that though (would also provide organization for my proposal above)
| divide 10 by {_num} | ||
| """) | ||
| @Since("INSERT VERSION") | ||
| public class EffOperations extends Effect implements SyntaxRuntimeErrorProducer { |
skriptlang-automation
bot
added
needs reviews
| multiply {_timespan} by 3 | ||
| """) | ||
| @Example(""" | ||
| # Will error due to literal |
There was a problem hiding this comment.
I don't think we need to include invalid examples
| return node; | ||
| } | ||
|
|
||
| private void printArithmeticError(Class<?> left, Class<?> right) { |
There was a problem hiding this comment.
I would mark these methods as static if possible
| Operator operator, | ||
| Class<L> leftClass, | ||
| Class<R> rightClass, | ||
| Class<?>... possibleReturnTypes |
There was a problem hiding this comment.
I might call it allowedReturnTypes
| Object rightObject = right.getSingle(event); | ||
| if (rightObject == null) { | ||
| error("Cannot operate with a null object."); | ||
| return; | ||
| } | ||
| if (left.isSingle() && left.getSingle(event) == null) { | ||
| error("Cannot operate on a null object."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not sure if we'd want runtime errors or not for null values (would like to hear what others think). Some operations/types have default values... do we need to be checking those here?
Description
This PR aims to add effects for multiplication, division and exponentiation operations, allowing users to perform an arithmetic operation on an variable, single or list. Rather than doing, for example
set {_int} to {_int} * 2Does not work for literals.
Target Minecraft Versions: any
Requirements: none
Related Issues: #7763